-
Notifications
You must be signed in to change notification settings - Fork 8k
phar: use SOURCE_DATE_EPOCH to build PHAR when available
#20570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: PHP-8.3
Are you sure you want to change the base?
Conversation
| if (tloc) { | ||
| *tloc = ts; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the API of this function:
- it returns a value?
- it overwrites a reference, if given?
- it uses an environment variable, if given?
I'd say that a rename (for clarity) would be a good idea, and the parameter can perhaps be always gone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should target master as it's a new feature. It should also get a test ;)
Also this is not your code, so you should get permission from zeroware and get his proper credits in (e.g. Co-authored-by tag)
|
|
||
| static inline time_t source_date_epoch_time(time_t *tloc) | ||
| { | ||
| const char *sde; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge the declarations and assignments, we no longer do C89
| return PHAR_G(cached_fp)[entry->phar->phar_pos].manifest[entry->manifest_pos].offset; | ||
| } | ||
|
|
||
| static inline time_t source_date_epoch_time(time_t *tloc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As pointed out by Ocramius, this argument seems not useful.
|
|
||
| tsrm_env_lock(); | ||
| sde = getenv("SOURCE_DATE_EPOCH"); | ||
| ts = (sde) ? strtoul(sde, NULL, 10) : time(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ts = (sde) ? strtoul(sde, NULL, 10) : time(0); | |
| ts = (sde) ? strtoul(sde, NULL, 10) : time(NULL); |
| time_t ts; | ||
|
|
||
| tsrm_env_lock(); | ||
| sde = getenv("SOURCE_DATE_EPOCH"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use php_getenv, which will do the right thing on Windows and also take care of locking.
Hello,
I recently discovered the project https://stagex.tools/ and out of curiosity I checked how the PHP package (https://codeberg.org/stagex/stagex/src/branch/staging/packages/core/php-zts) was built. This is how I discovered those two potential interesting patches (credits to @zeroware) that could easily be merged in the PHP distribution.
I proposed the adoption of those patches to the Nix community as well at NixOS/nixpkgs#463814
I hope the branch
PHP-8.3is the correct one, let me know if there's anything to change.Links:
This PR:
SOURCE_DATE_EPOCH(the file datetime) inside the PHAR only when it is available.